Nc account by id #8167

alphaprinz · 2024-06-26T03:37:30Z

Explain the changes

This PR removes references of account name from bucket json files.
The bucket_owner field is removed from bucket schema.
Principal in bucket policy is account id instead of account name.
In order to find account id by account name, a new dir root_accounts was added with symlinks from account name to account json.

Issues: Fixed #xxx / Gap #xxx

A Change to account name doesn't require updating bucket json files. bugs:
Account update with account name is not updating bucket_name.json file automatically #8080
NSFS | NC | Bucket owner & Principal of bucket policies should be ids and not email/name #7734 (though we still allow root account name as principal for backward compatibility).

Testing Instructions:

Same tests

Doc added/updated
Tests added

src/server/system_services/schemas/nsfs_bucket_schema.js

src/test/unit_tests/jest_tests/test_nc_nsfs_bucket_schema_validation.test.js

src/manage_nsfs/manage_nsfs_cli_utils.js

src/manage_nsfs/manage_nsfs_validations.js

src/sdk/bucketspace_fs.js

shirady · 2024-06-27T06:53:35Z

src/sdk/bucketspace_fs.js

+            const account_config_path = this._get_account_config_path(bucket.owner_account);
+            data = (await nb_native().fs.readFile(this.fs_context, account_config_path)).data;
+            const account = JSON.parse(data.toString());
+            nsfs_schema_utils.validate_account_schema(account);
+            is_valid = await this.check_bucket_config(bucket);
+            if (!is_valid) {
+                dbg.warn('BucketSpaceFS: account linked to bucket is not valid: ', name);
+            }


What is the purpose of this addition?
Please also add in comments.

Once we had the account name in the bucket json, now we don't.
In order to get the account name, I need to go through the same code as reading the bucket json.

I don't understand why to add an account schema check when we get its name.
And it seems that you check the bucket config twice:
(line 152) let is_valid = await this.check_bucket_config(bucket);
(line 160) is_valid = await this.check_bucket_config(bucket);

It's the same reasoning as checking the bucket schema when getting bucket data. I just went with the pattern already in the code.
Regarding the double is_valid, it's mistake, thanks for noticing.

I'm not sure you need to do it...
Imagine that something went wrong with the account (somehow it contains a property not in the schema), and you are trying to read it bucket and you will get an error on validation of the account (although it is not related to reading the bucket).

Having a problem in any json schema is a valid reason for a request to fail, specifically if that json contains information the request is supposed to return.

I think we should not fail a request in bucketspace if the account config that created it somehow is not valid.

Let's say we have:

an account (root account): account1

the account owns a bucket: bucket1

it has a bucket policy for all S3 actions for account1 and account2 (additional root account).

account2 sends requests on bucket bucket1 (he has the permission, so all the requests succeeded).

somehow account1 schema became invalid

what should happen to account2' requests?
In my opinion, his requests should still be succeeded...

src/sdk/bucketspace_fs.js

src/cmd/manage_nsfs.js

src/endpoint/s3/s3_rest.js

src/test/unit_tests/test_s3_bucket_policy.js

src/cmd/manage_nsfs.js

shirady · 2024-07-02T05:40:13Z

src/manage_nsfs/manage_nsfs_validations.js

    if (action === ACTIONS.ADD || action === ACTIONS.UPDATE) {
        if (action === ACTIONS.ADD) native_fs_utils.validate_bucket_creation({ name: data.name });
        if (action === ACTIONS.UPDATE && !_.isUndefined(data.new_name)) native_fs_utils.validate_bucket_creation({ name: data.new_name });

-        if (action === ACTIONS.ADD && _.isUndefined(data.bucket_owner)) throw_cli_error(ManageCLIError.MissingBucketOwnerFlag);
+        if (action === ACTIONS.ADD && _.isUndefined(data.owner_account)) throw_cli_error(ManageCLIError.MissingBucketOwnerFlag);


With the new code changes it seems to fail in get_bucket_owner_account with an undefined name and not with the error MissingBucketOwnerFlag.

I'm not sure I'm with you. Where do you see this failure? In what scenario? A test?
Are you talking about get_bucket_owner_account in fetch_bucket_data()?

I think I meant for a manual test that you can run with creating a bucket without passing the ---owner flag, which error do you see? (In this line it should be MissingBucketOwnerFlag, but I saw something else as mentioned).

I see. Since fetch_bucket_data() is before validate_bucket_args(), I've added a check for it in fetch_bucket_data().

I think we are trying to validate as early as we can, maybe you can do it before the fetch_bucket_data in a function under validate_input_types?
I think it might be a bit confusing to see validations in the fetch_bucket_data.

docs/NooBaaNonContainerized/Configuration.md

docs/NooBaaNonContainerized/GettingStarted.md

docs/NooBaaNonContainerized/NooBaaCLI.md

shirady · 2024-07-28T12:58:44Z

src/sdk/accountspace_fs.js

@@ -351,7 +362,7 @@ class AccountSpaceFS {
        try {
            const requesting_account = account_sdk.requesting_account;
            const access_key_id = params.access_key;
-            const requester = this._check_if_requesting_account_is_root_account_or_user_om_himself(action,
+            this._check_if_requesting_account_is_root_account_or_user_om_himself(action,


Why do you ignore the return value?

If we're going by access key, I don't need to deal with names.
Unless I'm missing sth?

shirady · 2024-07-28T13:00:56Z

src/sdk/accountspace_fs.js

@@ -402,7 +406,7 @@ class AccountSpaceFS {
        try {
            const requesting_account = account_sdk.requesting_account;
            const access_key_id = params.access_key;
-            const requester = this._check_if_requesting_account_is_root_account_or_user_om_himself(action,


We have the option for user-name as optional, let's see this change after merging PR #8233.

src/sdk/accountspace_fs.js

src/test/unit_tests/jest_tests/test_nc_nsfs_account_cli.test.js

src/test/unit_tests/jest_tests/test_nc_nsfs_bucket_cli.test.js

src/sdk/accountspace_fs.js

guymguym · 2024-07-30T12:00:12Z

src/endpoint/s3/s3_rest.js

+    const account_identifier_name = req.object_sdk.nsfs_config_root ? account.name.unwrap() : account.email.unwrap();
+    const account_identifier_id = req.object_sdk.nsfs_config_root ? account._id : undefined;
+    //In old buckets (until 5.17), system_owner is account name (sensitive string).
+    //In new buckets (since 5.18), system_owner is account id (string).
+    const system_owner_equatable = system_owner.unwrap ? system_owner.unwrap() : system_owner;
+    const is_system_owner =
+        (account_identifier_name === system_owner_equatable) ||
+        (account_identifier_id && account_identifier_id === system_owner_equatable);

    // @TODO: System owner as a construct should be removed - Temporary
    if (is_system_owner) return;

    const is_owner = (function() {
        if (account.bucket_claim_owner && account.bucket_claim_owner.unwrap() === req.params.bucket) return true;
-        if (owner_account && owner_account.id === account._id) return true;
-        if (account_identifier === bucket_owner.unwrap()) return true;
+        if (req.object_sdk.nsfs_config_root && account._id === owner_account.id) return true; // NC NSFS case
+        if (!req.object_sdk.nsfs_config_root && account_identifier_name === bucket_owner.unwrap()) return true;


I don't think that s3 rest api should look at nsfs_config_root at all, this is why we have an AccountSpace and BucketSpace interfaces. It seems that the use of this variable somehow has spread around in the code (even into ssl_utils god forbid). Instead, the code that does not directly writes/reads from the config dir should just look at the actual bucket and account config structures it received from read_bucket_sdk_policy_info and requesting_account, and use their information to make the ownership checks.

Generally I agree, but practically I went with the existing-
const account_identifier = req.object_sdk.nsfs_config_root ? account.name.unwrap() : account.email.unwrap();

I can try to remove what I have added.

I looked at it, and I don't see a quick/simple solution to differentiate between by-name or by-id ownership (in s3_rest).
If there's something I'm missing please let me know.
I can go by whether account_identifier_id is defined to reduce the foot print of req.object_sdk.nsfs_config_root in the code (but it's equivalent).

docs/NooBaaNonContainerized/NooBaaCLI.md

src/manage_nsfs/manage_nsfs_help_utils.js

src/manage_nsfs/manage_nsfs_cli_utils.js

shirady · 2024-07-30T12:10:37Z

src/manage_nsfs/manage_nsfs_cli_utils.js

@@ -168,7 +191,7 @@ async function get_options_from_file(file_path) {
 * @param {object[]} access_keys
 */
 function has_access_keys(access_keys) {
-    return access_keys.length > 0;
+    return access_keys.length > 0 && access_keys[0].access_key;


I'm not sure we want it, because the scenario of the access_keys object to be {access_key: undefined, secret_key: undefined} should not be a real data in a config, it is "by-product" of one of the functions.
In PR #8238 I'm suggesting to change it without changing this function.

Either way is fine by me.

src/sdk/bucketspace_fs.js

src/sdk/accountspace_fs.js

shirady · 2024-07-31T13:17:45Z

src/endpoint/s3/s3_rest.js

+    const account_identifier_id = req.object_sdk.nsfs_config_root ? account._id : undefined;
+    //In old buckets (until 5.17), system_owner is account name (sensitive string).
+    //In new buckets (since 5.18), system_owner is account id (string).
+    const system_owner_equatable = system_owner.unwrap ? system_owner.unwrap() : system_owner;


Are you sure about this line? system_owner.unwrap? (did you mean the function unwrap()?)
Are you trying to check if it is an ID or a sensitive string?

Yes. Sometimes it is a sensitive string, but in nc it's an id.
I think system owner is going to be removed anyway, no?

Yes, I have PR #8192 to remove the system owner (was not reviewed yet).

But still system_owner.unwrap will be always undefined (because it searches for the property unwrap in the object system_owner), better write the condition in the way you want, for example:
some_var instanceof SensitiveString ? some_var.unwrap() : some_var;

shirady · 2024-07-31T13:40:51Z

src/sdk/accountspace_fs.js

-            } else {
-                const requested_account_encrypted = await nc_mkm.encrypt_access_keys(requested_account);
-                const account_string = JSON.stringify(requested_account_encrypted);
-                nsfs_schema_utils.validate_account_schema(JSON.parse(account_string));
-                await native_fs_utils.update_config_file(this.fs_context, this.accounts_dir,
-                    account_config_path, account_string);


Could you please explain this change?

Looks like I want to update the json whether name was changed or not.

shirady · 2024-07-31T13:46:31Z

src/sdk/accountspace_fs.js

+    async _update_account_config_new_username(action, user_id, old_username, new_username, requesting_account) {
+        const root_account_name = await this._get_root_account_name(requesting_account);
+        await this._check_username_already_exists(action, new_username, root_account_name);
+        const root_account_old_name = get_symlink_config_file_path(this.root_accounts_dir, old_username, root_account_name);


I think I'm missing something, why there is a change in the root account name in the general code of updating the username?
Could you please add comments of the steps?

For iam account name change, the root account name does not change.
If you change the name of an iam account, you need to update its by-name symlink.
Part of the by-name symlink is the root account name.
Maybe the params names are confusing.

shirady · 2024-07-31T13:50:33Z

src/endpoint/s3/s3_rest.js

+    const account_identifier_id = req.object_sdk.nsfs_config_root ? account._id : undefined;
+    //In old buckets (until 5.17), system_owner is account name (sensitive string).
+    //In new buckets (since 5.18), system_owner is account id (string).
+    const system_owner_equatable = system_owner.unwrap ? system_owner.unwrap() : system_owner;


Yes, I have PR #8192 to remove the system owner (was not reviewed yet).

But still system_owner.unwrap will be always undefined (because it searches for the property unwrap in the object system_owner), better write the condition in the way you want, for example:
some_var instanceof SensitiveString ? some_var.unwrap() : some_var;

src/sdk/accountspace_fs.js

previously known as "account by id" Main change is that accounts/ dir now has <account_id>.json instead of <account_name>.json file. By-name indexing is in new root_accounts/<root_account_name>/<iam_account_name>.symlink. Signed-off-by: Amit Prinz Setter <[email protected]>

@alphaprinz

This PR is based on @alphaprinz noobaa#8167 PR. 1. In s3_rest under the function authorize_request_policy rename account_identifier to account_identifier_name and add account_identifier_id. 2. In bucketspace_fs where we use put_bucket_policy and in manage_nsfs_validations where we validate the bucket policy use is_account_exists_by_principal that accepts both account name and id. 3. Both in authorize_request_policy (s3_rest) and has_bucket_action_permission (bucketspace_fs) check the principal by ID first and then the principal by name. Signed-off-by: shirady <[email protected]>

nimrod-becker · 2024-09-04T06:48:24Z

superseded by
#8326 #8289 #8279 #8280 #8312

@alphaprinz

This PR is based on @alphaprinz noobaa#8167 PR. 1. In s3_rest under the function authorize_request_policy rename account_identifier to account_identifier_name and add account_identifier_id. 2. In bucketspace_fs where we use put_bucket_policy and in manage_nsfs_validations where we validate the bucket policy use is_account_exists_by_principal that accepts both account name and id. 3. Both in authorize_request_policy (s3_rest) and has_bucket_action_permission (bucketspace_fs) check the principal by ID first and then the principal by name. Signed-off-by: shirady <[email protected]>

pull-request-size bot added the size/L label Jun 26, 2024

alphaprinz requested review from guymguym, shirady, romayalon and naveenpaul1 June 26, 2024 03:37

alphaprinz changed the title ~~Nc account by~~ Nc account by id Jun 26, 2024

alphaprinz force-pushed the nc_account_by_id branch 3 times, most recently from 2a5df3c to d860c93 Compare June 27, 2024 05:28

shirady reviewed Jun 27, 2024

View reviewed changes

alphaprinz force-pushed the nc_account_by_id branch 6 times, most recently from 2f227ae to da61744 Compare June 27, 2024 22:21

pull-request-size bot added size/XL and removed size/L labels Jun 28, 2024

alphaprinz force-pushed the nc_account_by_id branch 9 times, most recently from 2284ee4 to 2ca3cd6 Compare July 1, 2024 17:51

shirady reviewed Jul 2, 2024

View reviewed changes

alphaprinz force-pushed the nc_account_by_id branch 2 times, most recently from 3e16ef7 to 5cb4178 Compare July 3, 2024 18:34

shirady reviewed Jul 28, 2024

View reviewed changes

alphaprinz force-pushed the nc_account_by_id branch from 61f6bca to e8805b6 Compare July 29, 2024 04:18

shirady reviewed Jul 29, 2024

View reviewed changes

src/sdk/accountspace_fs.js Outdated Show resolved Hide resolved

alphaprinz force-pushed the nc_account_by_id branch 2 times, most recently from cfdcb96 to 3a6b230 Compare July 30, 2024 05:50

alphaprinz requested a review from shirady July 30, 2024 05:50

alphaprinz force-pushed the nc_account_by_id branch from 3a6b230 to 9041f79 Compare July 30, 2024 05:53

guymguym requested changes Jul 30, 2024

View reviewed changes

shirady reviewed Jul 30, 2024

View reviewed changes

alphaprinz force-pushed the nc_account_by_id branch 4 times, most recently from 5a7b7ae to 4be39ac Compare July 30, 2024 23:41

shirady reviewed Jul 31, 2024

View reviewed changes

alphaprinz force-pushed the nc_account_by_id branch 6 times, most recently from 5568f50 to 834d49a Compare July 31, 2024 20:42

alphaprinz force-pushed the nc_account_by_id branch from 834d49a to 45ae15a Compare July 31, 2024 21:28

This was referenced Aug 12, 2024

NC | Config Dir Restructure #8279

Merged

NC | Bucket Owner Removal #8289

Merged

shirady mentioned this pull request Aug 20, 2024

NC | NSFS | Bucket Policy With Principal as account ID #8280

Merged

2 tasks

nimrod-becker closed this Sep 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nc account by id #8167

Nc account by id #8167

alphaprinz commented Jun 26, 2024 •

edited

Loading

shirady Jun 27, 2024

alphaprinz Jul 3, 2024

shirady Jul 7, 2024

alphaprinz Jul 8, 2024

shirady Jul 17, 2024

alphaprinz Jul 19, 2024

shirady Jul 21, 2024

shirady Jul 2, 2024

alphaprinz Jul 3, 2024

shirady Jul 7, 2024

alphaprinz Jul 8, 2024

shirady Jul 17, 2024

shirady Jul 28, 2024

alphaprinz Jul 29, 2024

shirady Jul 28, 2024

guymguym Jul 30, 2024

alphaprinz Jul 30, 2024

alphaprinz Jul 30, 2024

shirady Jul 30, 2024

alphaprinz Jul 30, 2024

shirady Jul 31, 2024

alphaprinz Jul 31, 2024

shirady Jul 31, 2024

shirady Jul 31, 2024

alphaprinz Jul 31, 2024

shirady Jul 31, 2024

alphaprinz Jul 31, 2024

shirady Jul 31, 2024

nimrod-becker commented Sep 4, 2024

Nc account by id #8167

Nc account by id #8167

Conversation

alphaprinz commented Jun 26, 2024 • edited Loading

Explain the changes

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimrod-becker commented Sep 4, 2024

alphaprinz commented Jun 26, 2024 •

edited

Loading